Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed minifier for Windows users #15893

Closed
wants to merge 2 commits into from

Conversation

9larsons
Copy link
Contributor

Resolves #14278.

Updated Minifier to use only forward slashes when specifying globs regardless of platform.

PR #15807 has some additional history. After more testing - and despite globalyzer (dependency of tiny-glob) failing on Windows - tiny-glob does seem to work on Windows as it indicates when you specify a glob using forward slashes. I suppose I ended up going too deep with the initial troubleshooting and missed the more obvious piece here.

This should alleviate your concerns about performance hits from changing packages, although I still feel there would be no impact moving to node-glob as we're fully aware of the directory of the files we want here so no crawling is necessary.

closes TryGhost#14278
Updated Minifier to use only forward slashes when specifying globs regardless of platform.
@9larsons
Copy link
Contributor Author

I just remembered I had a fix for one of the tests in my other prior commits, so I'll take a look at that tomorrow and get that added here. As I recall, one of the tests was a false positive.

@9larsons
Copy link
Contributor Author

9larsons commented Nov 29, 2022

Ok, now it should be good. One of the tests wasn't truly checking that it handled a missing file. and tiny-glob doesn't have any option to error when nothing is returned from the pattern so I had to shift around some of the error handling.

@ErisDS ErisDS self-assigned this Jan 23, 2023
@daniellockyer
Copy link
Member

Closing as stale for now but please re-open if desired 🙂

9larsons added a commit that referenced this pull request Oct 15, 2024
ref #15893

This got closed as stale long ago. Resurrecting this as it's a simple change and will help our Windows-based theme devs in particular.
9larsons added a commit that referenced this pull request Nov 21, 2024
ref #15893

This got closed as stale long ago. Resurrecting this as it's a simple
change and will help our Windows-based theme devs in particular.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot generate assets (cards, admin-auth, etc) on local windows install
3 participants